Minor comment: this make it impossible to compile the library without -fPIC flag. An alternative that still defaults to enable -fPIC unless someone explicitly disable it is to add:

if (NOT DEFINED CMAKE_POSITION_INDEPENDENT_CODE)
  set(CMAKE_POSITION_INDEPENDENT_CODE ON)
endif()

before the line add_library(${PROJECT_NAME} ${serial_SRCS})

Yes I agree this shouldn’t be merged as is with this it’s too invasive. I picked this out of the ros2 branch we’ve been using.

See comment above.

If you build the library as a shared library you don't need this.

I picked two commits from @leamas PR #231 (to set the so version) and then set BUILD_SHARED_LIBS ON.

Then tested that out of the box it works fine in a colcon workspace...
But if you do colcon build --cmake-args "-DBUILD_SHARED_LIBS=OFF" then you'll get back to ld errors

--- stderr: robotiq_driver                             
/usr/bin/ld: /home/alex/ros/h/robotiq/install/serial/lib/libserial.a(unix.cc.o): warning: relocation against `_ZTVN6serial15SerialExceptionE' in read-only section `.text._ZN6serial15SerialExceptionD2Ev[_ZN6serial15SerialExceptionD5Ev]'
/usr/bin/ld: /home/alex/ros/h/robotiq/install/serial/lib/libserial.a(serial.cc.o): relocation R_X86_64_PC32 against symbol `_ZTVN6serial6SerialE' can not be used when making a shared object; recompile with -fPIC
/usr/bin/ld: final link failed: bad value

If I additionally add the suggestion from @traversaro then it seems to work in both cases:
colcon build --cmake-args "-DBUILD_SHARED_LIBS=OFF" will build a ./install/serial/lib/libserial.a which can be used without linking errors

@wjwwood any opinion or preference on this?

Why such an old version? I'm not sure what non-EOL platforms these days ship anything older than 3.16.

RHEL8 ships with 3.16, the oldest version of CMake in a distro currently supported by a tier 1 or 2 versions of ROS to give you more context. @ChrisThrasher, do you mind helping explain the benefits of using a newer version of CMake and provide some references in case @moriarty would like more information about this recommendation?

The main benefit is that it means you opt-in to many new and improved default behaviors (what CMake calls "policies") that have been added between version 3.5 and 3.16.

https://cmake.org/cmake/help/latest/manual/cmake-policies.7.html

On top of that, nobody is actually using 3.5 or testing with 3.5 so while we can claim this script doesn't use features added after 3.5 we can't be sure that unless we add a CI job that uses 3.5. CMake will let you use newer features even if your minimum version is very old.

I took 3.5 because I got a CMake warning that said something and suggested using at least 3.5

Also when I saw that warning, I just cherr-picked because it was on the other open PR which is what is being released into debian as libcxx-serial-dev : c9da89d but when I tried to use that package I had to add some define around the includes of serial (WIP testing here: PickNikRobotics/ros2_robotiq_gripper#22)

7b7f62b I've set this to 3.16

Why have this variable instead of just hardcoding this value a few lines down where it's used?

Similar to the recommendation to not use the PROJECT_NAME variable, biasing away from creating variables in cmake files makes them more boring and easier to read. CMake is a hard language to read and get correct and the more boring you can make it the nicer you are to yourself and others in the future.

One example where I've gone the other way from this recommendation though is in ros2_control with our use of the THIS_PACKAGE_INCLUDE_DEPENDS variable. You can find an example of that here: https://github.com/ros-controls/ros2_controllers/blob/f059022622e12115d006e0677ca2e312eed78858/joint_trajectory_controller/CMakeLists.txt#L8
The way I justify that variable is it greatly shortens the cmake file and cuts down on copy-paste errors in the dependencies that are specified for the project. I do not think your use of PROJ_SOVERSION is that case because it is just the number 1 and using the variable makes the cmake file longer (not to count the declaration of this variable) and adds indirection in every place where you use it. Is this a helpful explanation?

Makes sense. I will remove the PROJ_SOVERSION and just use 1.

all of the PROJECT_NAME variables come from cherry-pick from #231 and a later change in that PR makes it possible to have an alternative name for this library cxx-serial 5f1ab38 which is what @leamas released into the debian packages.

There are so many diverging forks of this library :'(

But I agree and will change PROJECT_NAME back to serial for readability in a separate commit so if @wjwwood or @leamas chime in to this discussion it could always be reverted.

PROJ_SOVERSION is actually crucial. It defines the binary compatibility level, and as such has been 1 so far. However, all serious packaging relies on a defined SOVERSION, without it all dependent packages will need to be recompiled for each and every change in the this lib, also the ones which are downwards compatible.

EDIT: I actually explained that in the commit, see below

That the library actually needs a new name is a reason, perhaps the only compelling one, to make a friendly fork. Forking is of course a bad thing, but perhaps the correct way to get out of the rabbit hole created by the too general serial/serial.h. It would make it easier to make big change while keeping the current lib as-is, for sure.

Issues could be moved to a fork without too much hassle, either in the GUI one-by-one or on the command line using a bulk transfer.

@leamas Nobody is asking to stop specifying the SOVERSION. We're saying that we don't need an extra variable to hold the value 1. That value can be hardcoded in the set_target_properties call where it's needed.

Keeping the name is about traceability. Put it this way: if you replace SOVERSION with a an explicit constant like 1 or 2 it will need a comment explaining what it's all about. Such needs for comments is a sure sign of bad design

Yes, cmake is a from time to time a hard language. However, this makes it even more important to stick to standard programming principles, one of which being to declare constants rather than using literal values. That is, SOVERSION is to be preferred to literal whatever.

Would to you propose a similar change to a C/C++ program? If you did, what do think the reaction would be?

How is

set_target_properties(${PROJECT_NAME} PROPERTIES
     VERSION ${PROJECT_VERSION}
     SOVERSION ${PROJ_SOVERSION}
     PUBLIC_HEADER "${serial_HEADERS}"
 )

better than

set_target_properties(${PROJECT_NAME} PROPERTIES
    VERSION ${PROJECT_VERSION}
    SOVERSION 1
    PUBLIC_HEADER "${serial_HEADERS}"
)

How is SOVERSION 1 unclear whatsoever? No comments are required to understand what's going on.

You don't need to specify headers as source files if you don't want to.

cmake: Add defined so-version and public header to lib.
Adding a so-version means defining an ABI level. This level is decoupled
from the ordinary version, even a major version change doesn't
necessarily mean that the so-version should change (and thus have all
dependencies to be rebuilt).

Adding the public header to clarify the setup.

Gbp-Pq: Name 0002-cmake-Add-defined-so-version-and-public-header-to-li.patch

This was added here ed0e389 which I cherry-picked 61da1e2

v8stdint.h is actually problematic. It is now long time since I worked with this, but IIRC I think the situation is:

• v8stdint.h is basically a band-aid for systems not having stdint.h available, notably windows.
• The definitions in v8stdint.h clashes with other files on some systems.

That is, the correct solution would be to use cmake to probe for stdint.h and only include v8stdint.h if it's required. Needs to be double-checked, though. If it is indeed required anyway, cmake should probe for v8stdint.h and only include it if it's not available.

On Fedora, v8stdint.h is part of the v8 package.

Of course, this is not only about including v8stdint.h or not in the package. It is also about conditionalizing the source files so they include the correct file, stdint.h or v8stdint.h. I made a quick hack for this in the Debian package, but it probably needs polish before being merged into the upstream package here.

You don't need to specify headers as source files if you don't want to.

This is IMHO the wrong way to go. Recommended cmake best practice is certainly to include all used files in the project.

I have seen a numbner of cmake projects, none of without the headers in the source lists. In this case, I think that you need to come up with some trustworthy source recommending not to include the headers.

You have already started the list of functionality which breaks without a complete list of source files, including headers. I can assure you there is more, and that we currently not use it is not much of an argument.

BTW: how would you install headers not being part of the sources?

I think that you need to come up with some trustworthy source recommending not to include the headers.

I'd argue the burden of proof is on the person who wants to write more code.

You have already started the list of functionality which breaks without a complete list of source files

Nothing breaks for Visual Studio users. It's merely an inconvenience. Visual Studio users will also ask you use source_groups in spite of that not being strictly necessary.

how would you install headers not being part of the sources?

Same way as always (prior to FILE_SETs). You install the include directory or something similar. Adding headers as source files does not affect installation.

I simply don't have time for this, sorry. It's up to the maintainer to judge.

My experience has been that adding headers is not required at all. It does help some editors who want to list "files used in targets" without involving a compiler, but that's all. Whether or not it's recommended by CMake, I'm not sure about.

That being said, I don't care if they are included in this way. If it helps someone and they contribute it then I think it's fine.

Fair enough. But, as you said, cstdint.h is the better alternative to v8stdint.h making this to be serial.h only

It aids in readability and grepability if you simply spell out the variable name.

👍 to this recommendation as it makes the cmake file much easier to read and understand. One of the hardest parts of cmake is reading the config files.

I agree here, but I'll wait for @wjwwood to chime in on this PR in general.

I am concerned this PR gets too big and goes the way of any of the other open PRs

It's fine with me, but it deviates from what it common in the ROS ecosystem.

It's fine with me, but it deviates from what it common in the ROS ecosystem.

Not only the ROS ecosystem, but the whole cmake community I would say. For that reason I think this could be left as-is

@ChrisThrasher wrote

#285 (comment)

No. This should IMHO be done like in e1dabd8, taking into account whether to include v8stdint.h or not.

If we're adding a config module, we should write a config module that exports a target instead of doing the old school approach of setting some variables that downstream projects have to use. That means we need to create an export set for the library target and install that export set then this file becomes a one liner that will look something like this:

include(${CMAKE_CURRENT_LIST_DIR}/serial-targets.cmake)

@ChrisThrasher can you point to helpful documentation and examples on how to do this well?

https://github.com/ChrisThrasher/argon/blob/9a2982c5471f06ca1d97f8d2d8a1aab3055ba456/CMakeLists.txt#L29-L42

I'm biased but this installation code I wrote I quite like. It's goes above and beyond to be as robust and idiomatic as possible. You don't need to copy everything it does but it should do a good job laying our the basic steps.

1. Install all public headers. I do this by installing the entire include/ directory.
2. Create export set. This happens to only include a single library target but may include more if need be. It uses variables from the GNUInstallDirs module to ensure all files are installed in platform-appropriate locations that can be easily modified by the user should they want to install the library in a different location. I include the package name and version in the install tree but you can omit that.
3. Install said export set. This is where you can (and should) specify a namespace for all exported targets.
4. Create a version config file. This ensures that users can ask for a specific version and that will be checked against the version that is installed.
5. Install the config module and the version config file.

instead of doing the old school approach of setting some variables that downstream projects have to use.

This is not old-school. The normal usage pattern would be something like below, assuming that the serial library lives in a subdirectory

include(libs/serial)
target_link_libraries(${PROJECT_NAME} PRIVATE wjserial::serial)

This would just require a one-liner in CMakeLists.txt defining a ALIAS target. It relies on cmake's support for transitive dependencies. Adding such a target would simplify documentation and overall usage a lot.

Yes, that's what I'm saying. We're in agreement. The variable-based approach is old school because targets are the modern way of doing things.

It aids in readability and grepability if you simply spell out the variable name.

This is actually a personal preference... I actually agree on the personal level, but let's face it: the use of ${PROJECT_NAME} is common enough in the cmake community to leave in place.

Don't mistake commonality for it being a good idea. Lots of bad practices are still popular but that should not be used as an argument in favor of perpetuating bad ideas.

This might be a good time to change to exported targets, and also consider putting the headers in a separate subfolder, which we've been doing in ROS 2 for a while now to promote better header isolation in a merged install space.

I.e. we'd put the headers in include/serial/serial and the exported include flags would be like -I/path/to/include/serial with the code still doing #include "serial/serial.h".